Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add support for policy tags #77

Merged
merged 22 commits into from
May 18, 2020
Merged

feat: add support for policy tags #77

merged 22 commits into from
May 18, 2020
+278 −5

Conversation

shollyman
Copy link
Contributor

@shollyman shollyman commented Apr 16, 2020 •
edited
Loading

State: unit testing done
Pending: system tests

Fixes: #74

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 16, 2020
@shollyman
Copy link
Contributor Author

@plamut I'm not understanding the coverage failures from the most recent run, which don't appear to be related to this PR. Any insights?

Running session cover
Creating virtual environment (virtualenv) using python3.8 in .nox/cover
pip install coverage pytest-cov
coverage report --show-missing --fail-under=100
Name                                                Stmts   Miss Branch BrPart  Cover   Missing
-----------------------------------------------------------------------------------------------
google/cloud/bigquery/__init__.py                      57      0      0      0   100%
google/cloud/bigquery/_helpers.py                     237      0    120      0   100%
google/cloud/bigquery/_http.py                         13      0      0      0   100%
google/cloud/bigquery/_pandas_helpers.py              262      0    125      0   100%
google/cloud/bigquery/client.py                       642      2    272      0    99%   22-23
google/cloud/bigquery/dataset.py                      252      0     61      0   100%
google/cloud/bigquery/dbapi/__init__.py                30      0      0      0   100%
google/cloud/bigquery/dbapi/_helpers.py                77      2     42      0    98%   17-18
google/cloud/bigquery/dbapi/connection.py              15      0      2      0   100%
google/cloud/bigquery/dbapi/cursor.py                 128      2     36      0    99%   21-22
google/cloud/bigquery/dbapi/exceptions.py              11      0      0      0   100%
google/cloud/bigquery/dbapi/types.py                   22      0      0      0   100%
google/cloud/bigquery/encryption_configuration.py      28      0      4      0   100%
google/cloud/bigquery/enums.py                         31      0      2      0   100%
google/cloud/bigquery/exceptions.py                     2      0      0      0   100%
google/cloud/bigquery/external_config.py              314      0     30      0   100%
google/cloud/bigquery/job.py                         1373      0    236      0   100%
google/cloud/bigquery/magics.py                       192      0     69      0   100%
google/cloud/bigquery/model.py                        158      0     24      0   100%
google/cloud/bigquery/query.py                        238      0     72      0   100%
google/cloud/bigquery/retry.py                         13      0      4      0   100%
google/cloud/bigquery/routine.py                      195      0     30      0   100%
google/cloud/bigquery/schema.py                       114      0     42      0   100%
google/cloud/bigquery/table.py                        760      0    244      0   100%
tests/unit/__init__.py                                  0      0      0      0   100%
tests/unit/enums/__init__.py                            0      0      0      0   100%
tests/unit/enums/test_standard_sql_data_types.py       33      0      8      0   100%
tests/unit/helpers.py                                   8      0      0      0   100%
tests/unit/model/__init__.py                            0      0      0      0   100%
tests/unit/model/test_model.py                        125      0      0      0   100%
tests/unit/model/test_model_reference.py               72      0      0      0   100%
tests/unit/routine/__init__.py                          0      0      0      0   100%
tests/unit/routine/test_routine.py                    114      0      0      0   100%
tests/unit/routine/test_routine_argument.py            47      0      0      0   100%
tests/unit/routine/test_routine_reference.py           72      0      0      0   100%
tests/unit/test__helpers.py                           617      0      6      0   100%
tests/unit/test__http.py                               68      0      0      0   100%
tests/unit/test__pandas_helpers.py                    435      0     40      0   100%
tests/unit/test_client.py                            3238      4    104      4    99%   7449->7450, 7450, 7499->7500, 7500, 7541->7542, 7542, 7586->7587, 7587
tests/unit/test_dataset.py                            497      0     20      0   100%
tests/unit/test_dbapi__helpers.py                     116      0     10      0   100%
tests/unit/test_dbapi_connection.py                    73      0      0      0   100%
tests/unit/test_dbapi_cursor.py                       298      0     14      0   100%
tests/unit/test_dbapi_types.py                         18      0      0      0   100%
tests/unit/test_encryption_configuration.py            75      0      0      0   100%
tests/unit/test_external_config.py                    204      0      0      0   100%
tests/unit/test_job.py                               3432      0    132      0   100%
tests/unit/test_magics.py                             748      0      4      0   100%
tests/unit/test_query.py                              625      0      4      0   100%
tests/unit/test_retry.py                               38      0      0      0   100%
tests/unit/test_schema.py                             360      0      8      0   100%
tests/unit/test_signature_compatibility.py             20      0      0      0   100%
tests/unit/test_table.py                             2089      0    100      0   100%
-----------------------------------------------------------------------------------------------
TOTAL                                               18586     10   1865      4    99%

@plamut
Copy link
Contributor

plamut commented Apr 20, 2020 •
edited
Loading

@shollyman The coverage is aggregated across all unit test runs, and that final number is then checked (needs to be 100%).

There are several if-py2.7-else compatibility checks in the code, but since the Python 2.7 unit tests session encountered an error, it did not contribute to the total coverage and the coverage check failed.

It's not related to the changes here, however, as it also happens on master. Some dependencies do not build anymore on older Python versions, will investigate.

Update:
A new version of llvmlite (an indirect dependency) was released three days ago, breaking the installation on Python 2.7 and 3.5. A fix is available: #79.

plamut
plamut reviewed Apr 20, 2020
View reviewed changes
Copy link
Contributor

@plamut plamut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good. I had a few comments, but mostly nits - feel free to dismiss those if you find them too insignificant.

Adding a system test or two makes sense, indeed, awaiting that.

google/cloud/bigquery/schema.py Outdated Show resolved Hide resolved
google/cloud/bigquery/schema.py Outdated Show resolved Hide resolved
google/cloud/bigquery/schema.py Outdated Show resolved Hide resolved
google/cloud/bigquery/schema.py Show resolved Hide resolved
google/cloud/bigquery/schema.py Outdated Show resolved Hide resolved
google/cloud/bigquery/schema.py Outdated Show resolved Hide resolved
@shollyman
Copy link
Contributor Author

There's a problem with the integration testing on the datacatalog side, tracking this internally as 154544391

@shollyman
Copy link
Contributor Author

That said, it seems like you can avoid the integration and simply supply policy tags that match the defined resource pattern, so I'll add a quick test to do so and we can revisit once the datacatalog API issues are resolved.

plamut
plamut reviewed Apr 21, 2020
View reviewed changes
Copy link
Contributor

@plamut plamut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We missed one deepcopy, sorry for not mentioning it in the initial pass, while the rest looks good. Thanks for the quick update!

Looking forward to that quick test and we should be ready to merge then.

google/cloud/bigquery/schema.py Outdated Show resolved Hide resolved
plamut
plamut reviewed May 15, 2020
View reviewed changes
Copy link
Contributor

@plamut plamut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. The several comments I made during the review have already been fixed in subsequent commits, and what's left is an edge case that seems to not be covered by a test yet.

google/cloud/bigquery/schema.py Outdated Show resolved Hide resolved
google/cloud/bigquery/schema.py Outdated Show resolved Hide resolved
google/cloud/bigquery/schema.py Show resolved Hide resolved
google/cloud/bigquery/schema.py Outdated Show resolved Hide resolved
tests/unit/test_schema.py Show resolved Hide resolved
@shollyman
Copy link
Contributor Author

After wobbling a bit more, I decided that the right thing here was to make policytags immutable as it lives inside the immutable schemafield entities. Hence, ripped out the setter, and switch to/from api repr to convert between tuple and list forms.

plamut
plamut reviewed May 18, 2020
View reviewed changes
Copy link
Contributor

@plamut plamut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making the object immutable sounds sensible, and the changes look good. 👍

I just spotted a comment in one of the docstrings that appears to be redundant/misleading, but that's about it.

google/cloud/bigquery/schema.py Outdated Show resolved Hide resolved
@plamut plamut merged commit 38a5c01 into googleapis:master May 18, 2020
@quartzmo quartzmo mentioned this pull request Apr 20, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@plamut plamut plamut approved these changes

Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support policy tags in FieldSchema for column-level security
3 participants
@shollyman @plamut @googlebot